Skip to content

Conversation

@sirreal
Copy link
Member

@sirreal sirreal commented Oct 24, 2019

Node v12 is now LTS. Use it.

Updates node-sass as well so there are available binaries.

Blockers

@matticbot
Copy link
Contributor

@sirreal sirreal force-pushed the update/node-lts-12 branch from 2bc1975 to eb8b53b Compare October 24, 2019 14:51
@sirreal
Copy link
Member Author

sirreal commented Oct 24, 2019

This needs an update:

https://github.com/Automattic/wp-calypso/blob/eb8b53baf4bb033d75920eb6b6d339ca2b076be0/package.json#L214

Do we allow the last LTS for a while before requiring everyone to be on v12, or just make the switch?

https://docs.npmjs.com/files/package.json#engines

@sirreal sirreal self-assigned this Oct 24, 2019
@sirreal sirreal added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Build Framework labels Oct 24, 2019
@sirreal sirreal marked this pull request as ready for review October 24, 2019 14:55
@sirreal sirreal requested a review from a team as a code owner October 24, 2019 14:55
@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

@sirreal
Copy link
Member Author

sirreal commented Oct 24, 2019

FSE fails because it's package-lock uses the old node because it depends on a pinned calypso-build version 🤔

We could run that build on the older node version, or release another version of calypso-build after #35898 lands.

@sirreal
Copy link
Member Author

sirreal commented Oct 24, 2019

👋 @loremattei It looks like wp-desktop may have problems with Node 12.

electron-spellchecker isn't building.

@sirreal
Copy link
Member Author

sirreal commented Oct 24, 2019

We could run that build on the older node version

Not working.

or release another version of calypso-build after #35898 lands.

Seems we'll need to do this.

@blowery

This comment has been minimized.

@loremattei
Copy link
Contributor

Hey @sirreal! Thanks for the ping!

This requires an update to the Electron version used in WPDesktop. @nsakaimbo has already started to work on this and will be able to continue in a couple of weeks, but it's a non trivial task that may require some time because we currently have a huge tech debt (we are on Electron 1.7).

So, we'll probably need a B-plan in order to be able to continue to build the app in the meantime.

cc/ @belcherj

@sirreal sirreal force-pushed the update/node-lts-12 branch from 7b3c445 to 894637d Compare October 25, 2019 08:33
@sirreal sirreal requested review from a team as code owners October 25, 2019 08:33
@sirreal sirreal force-pushed the update/node-lts-12 branch from 894637d to 0d586ad Compare October 25, 2019 12:27
@blowery
Copy link
Contributor

blowery commented Oct 25, 2019

So, we'll probably need a B-plan in order to be able to continue to build the app in the meantime.

That might just be to relax engines for a while to continue to allow the 10.x LTS line. I don't think we're doing anything with the upgrade that precludes the old line...

@nsakaimbo
Copy link
Contributor

nsakaimbo commented Oct 25, 2019

That might just be to relax engines for a while

That might be worth a try, although not sure if the current spellchecker will play nice with that. TL;DR - if we change the version of the global node environment that WP-Desktop is compiled in (from 10.x to 12.x), it will try to fetch prebuilt binaries for the spellchecker that correspond to that version of Node. The problem is, we found out, the prebuilt spellchecker repo hasn't been updated since 2017, so compilation will fail because the prebuilt spellchecker doesn't exist for Node 12.x (yikes!). This will take some effort to unwind (and we've already made some progress).

However perhaps I'm misunderstanding your suggestion, and there's some other way we can work around it in the mean time?

Worst case, can we hold back the SHA of the version of Calypso we're using in WP-Desktop until the Electron upgrade is complete and these issues are resolved? cc: @loremattei @belcherj

@blowery
Copy link
Contributor

blowery commented Oct 25, 2019

However perhaps I'm misunderstanding your suggestion, and there's some other way we can work around it in the mean time?

I think maybe? I'm just saying that we could relax engines so that we can continue to allow Node@10. Not sure how that might play out with the .nvmrc and the build tooling though...

@sirreal
Copy link
Member Author

sirreal commented Oct 25, 2019

Automattic/wp-desktop#688 may let wp-desktop stay behind while wp-calypso moves ahead.

@gwwar
Copy link
Contributor

gwwar commented Oct 25, 2019

Maybe also cc @Automattic/i18n to double-check translation scripts. I recall we had some issues in the past on major version bumps.

@gwwar
Copy link
Contributor

gwwar commented Oct 25, 2019

the prebuilt spellchecker repo hasn't been updated since 2017

@nsakaimbo are there any other viable alternatives that are maintained?

@sirreal sirreal added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Oct 28, 2019
@sirreal sirreal force-pushed the update/node-lts-12 branch from 0d586ad to 0d30da0 Compare October 28, 2019 16:24
Node ^12.13.0
npm ^6.9.0
@sirreal
Copy link
Member Author

sirreal commented Oct 28, 2019

All tests are green here, time for testing and review 🚀

@belcherj
Copy link
Contributor

@gwwar There really aren't any. It may be a situation that we have to fork and update the spellchecker.

@belcherj
Copy link
Contributor

@nsakaimbo @gwwar This is good news though: atom/node-spellchecker#128

That's the package underneath that is failing.

@sirreal
Copy link
Member Author

sirreal commented Oct 31, 2019

@belcherj @gwwar As far as I can see we're free to land this since Automattic/wp-desktop#688 was merged. Is that accurate or are there remaining blockers?

All tests are green but I won't land this without some explicit approval.

@akirk
Copy link
Member

akirk commented Oct 31, 2019

npm run translate seems to be working fine so good to go from our @Automattic/i18n perspective.

@nsakaimbo
Copy link
Contributor

nsakaimbo commented Oct 31, 2019

@sirreal Have you verified that this branch can be built with wp-desktop, at least locally? Even though Automattic/wp-desktop#688 has landed, would be great to confirm that things are continuing to build as expected.

@gwwar

(re: spellchecker) are there any other viable alternatives that are maintained?

From my research, it seems no one else has even attempted to maintain an up-to-date prebuild fork of the spellchecker. We'll have to switch to using the spellchecker source in order to move forward with the Electron upgrade. Using prebuilds has proven highly impractical to maintain, so it's no surprise the project being relied on was effectively dropped - a little more context in this gist here.

@sirreal
Copy link
Member Author

sirreal commented Oct 31, 2019

@sirreal Have you verified that this branch can be built with wp-desktop, at least locally?

As part of Automattic/wp-desktop#688, I changed the Calypso submodule to use this branch (Automattic/wp-desktop@fd626ff) and the tests passed:

https://circleci.com/gh/Automattic/wp-desktop/47765

Copy link
Contributor

@blowery blowery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@simison
Copy link
Member

simison commented Oct 31, 2019

FYI @Automattic/jetpack-crew would be good to update also on Jetpack because so many folks work on both and would need to be switching between versions each time.

@sirreal sirreal merged commit 695dd85 into master Nov 4, 2019
@sirreal sirreal deleted the update/node-lts-12 branch November 4, 2019 09:20
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 4, 2019
sirreal added a commit that referenced this pull request Nov 4, 2019
@sirreal sirreal mentioned this pull request Nov 4, 2019
sirreal added a commit that referenced this pull request Nov 4, 2019
sirreal added a commit that referenced this pull request Nov 4, 2019
sirreal added a commit that referenced this pull request Nov 11, 2019
sirreal added a commit that referenced this pull request Nov 11, 2019
Second attempt upgrading to Node LTS 12 Erbium.

Original attempt in #37031

- Update root shrinkwrap
- Update app package locks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants